First contract version #1

bingen · 2018-06-22T17:33:39Z

No description provided.

Staking app registers locks per user, not globally.

Computed result was not being used because of memory variable usage.

Besides, own usage lock register was not being freed.

Closes #2.

Locks now have start date too. Modified events in tests. `unlockAndMoveTokens` renamed to `unlockPartialAndMoveTokens`.

izqui

Nice work here! Found some things that we should review

izqui · 2018-08-01T11:19:30Z

contracts/PLCR.sol

+
+    bytes32 constant public CREATE_VOTE_ROLE = keccak256("CREATE_VOTE_ROLE");
+
+    // TODO!!!


What needs to be done here?

izqui · 2018-08-01T14:16:03Z

contracts/PLCR.sol

+    using SafeMath64 for uint64;
+
+    IStaking public staking;
+    uint256 public voteQuorum;


Maybe a better name for this is passingThreshold or supportRequiredPct (as in Voting.sol) to better signal that it doesn't have anything to do with the proportion of holders that need to vote, but the relative forVotes / totalVotes

izqui · 2018-08-01T14:17:59Z

contracts/PLCR.sol

+
+    IStaking public staking;
+    uint256 public voteQuorum;
+    uint256 public minorityBlocSlash;


This property should be able to be modified, as it doesn't fundamentally change the properties of a vote, just the incentive for voters to be in the winning side.

izqui · 2018-08-01T14:19:25Z

contracts/PLCR.sol

+        mapping(address => UserVote) userVotes;
+        uint256 votesFor;
+        uint256 votesAgainst;
+        uint256 totalVotes;


If totalVotes is always for + against then it is better not to store it and just compute it every time.

Agreed! 👍

izqui · 2018-08-01T14:21:12Z

contracts/PLCR.sol

+     * @notice Create a new vote about "`_metadata`"
+     * @param _script EVM script to be executed on approval
+     * @param _metadata Vote metadata
+     * @return voteId Id for newly created vote


small typo: Id for the newly created vote

izqui · 2018-08-01T15:04:40Z

contracts/PLCR.sol

+        }
+
+        // unlock own tokens
+        staking.unlock(msg.sender, userVote.lockId);


Right now the app will only unlock if you reveal your vote. We need a way for people to unlock their tokens in these three cases:

They forgot to reveal their vote

They created a lock but never ended up committing a vote

They committed a vote and change their mind before the reveal period starts (in case they want to remove their vote or vote for the other option)

I think a good way to implement this would be for usedLocks to keep track of the voteId where the lock has been used. If it is 0 then it hasn't been used (would require moving the first index of votes to 1). By having the reference to the voteId where a given lockId we can have an removeLock() function will unlock depending on the case:

If the reveal period of the vote is over, directly unlock (as revealing would have deleted the reference from the lockId to the voteId). Otherwise revert.

Directly unlock if the voteId for a lockId is 0.

If the reveal period hasn't begun, remove the committed vote, return tokens from slashPool and unlock. Otherwise revert.

We need to think if there are any other scenarios in which tokens could be stuck forever in the contract. I think we should be okey with this + the ability to claim tokens on behalf of other addresses. But let's keep thinking about other edge cases.

Maybe the third case could be split to its own function as it has further consequences than just unlocking

Actually I just realized that users can unlock tokens in cases 1 and 2 because the lock is only required to be there during the reveal period. I still think we have to build the removeLock() function in case the start and end date of the locks are set incorrectly.

This realization brings me a different problem, which is that right now a staking lock can be unlocked by anyone outside of the locking period: https://github.com/aragon/aragon-apps/blob/b7fbaead3ad157db508b98092e8df73b9a6f6627/future-apps/staking/contracts/Staking.sol#L437 This is an issue as if you create a lock that starts right when the reveal period starts, someone could perpetually remove your locks before you have the chance to commit to a vote. I think this is an issue with Staking beyond the interaction with PLCR that we should solve.

Also while revealing we would need to check that the lock is still in place, because a valid lock at the time of commitment may get unlocked before reveal and given that we don't check the lock when revealing the vote, someone could double vote.

I think the best solution is requiring users to have a lock that started before the moment they commit a vote (rather than have the lock start time at the beginning of the reveal window) and allow them to remove locks from the PLCR app in those 3 cases, which still make tokens liquid at all times except during the reveal period until they reveal.

izqui · 2018-08-01T15:39:49Z

test/plcr.js

+      assert.isTrue(checkUnlockedPartial(receipt, voter1, app.address, lockId, slashPool))
+      assert.isTrue(checkMovedTokens(receipt, voter1, app.address, slashPool))
+      const userVote = await app.getUserVote.call(voteId, voter1)
+      assert.equal(userVote[0].toString(), lockId, "lockId should match")


Too similar to the previous test, could probably extract into a function and not repeat so much code?

izqui · 2018-08-01T15:42:25Z

contracts/PLCR.sol

+        staking.unlockPartialAndMoveTokens(msg.sender, _lockId, address(this), slashStake);
+        vote.slashPool = vote.slashPool.add(slashStake);
+
+        vote.userVotes[msg.sender] = UserVote({


We should check that the voter hasn't already committed before, as the second commitment will override the first commitment

Just brought this up :)

izqui · 2018-08-01T15:44:09Z

test/plcr.js

+      return { voteId: voteId, receipt: receipt }
+    }
+
+    const revealVote = async (voter, voteOption) => {


Should be named commitAndReveal vote for clarity of what the function is really doing.

izqui · 2018-08-01T15:51:33Z

test/mocks/StakingMock.sol

+import "@aragon/apps-staking/contracts/interfaces/IStaking.sol";
+
+
+contract StakingMock is IStaking {


Just a note: Love how this nice mock allows for easier unit testing of PLCR. We will need to do proper integration testing for all the user stories once we put everything together to ensure it all works as expected with real staking!

izqui · 2018-08-02T10:51:33Z

contracts/PLCR.sol

+     * @param _metadata Vote metadata
+     * @return voteId Id for newly created vote
+     */
+    function newVote(bytes _script, string _metadata) isInitialized external auth(CREATE_VOTE_ROLE) returns (uint256 voteId) {


Script is not saved and therefore it is never executed. PLCR should be a forwarder.

benstew · 2018-08-09T20:48:26Z

contracts/PLCR.sol

+        uint256 lockId;
+        uint256 stake;
+        bytes32 secretHash;
+        bool revealed;


Great call adding revealed. Thoughts on also including a field to determine if a user has committed a vote? ie

bool didCommit; /// indicates whether an address committed a vote

This can then also be leveraged as a secondary validation (sanity check) in the voting process to guard against double voting. Also takes some of the pressure off strictly relying on voteOption

benstew · 2018-08-09T20:48:35Z

contracts/PLCR.sol

+        uint64 revealEndDate;
+        bool result;
+        bool computed;
+        string metadata;


Thoughts on splitting metadata into it's own separate struct? My concern is that the field currently enables a wide spectrum of possible input. A metadata struct would enable a little more structure.

struct Metainformation { string title; string description; }

This is assuming we can align on fields, if not.. I don't think it's necessary

benstew · 2018-08-09T20:48:51Z

contracts/PLCR.sol

+        vote.revealEndDate = vote.commitEndDate.add(revealDuration);
+        vote.metadata = _metadata;
+
+        NewVote(voteId);


Should we be emitting an event here? Also, I think we have a couple other unused events.

benstew · 2018-08-09T20:49:12Z

contracts/PLCR.sol

+        Vote storage vote = votes[_voteId];
+        UserVote storage userVote = votes[_voteId].userVotes[msg.sender];
+
+        // check reveal period


FYI - can use the didCommit field here that I mentioned above as well.

require(!didCommit(_address, _pollID));

benstew · 2018-08-09T20:49:21Z

contracts/PLCR.sol

+        UserVote storage userVote = votes[_voteId].userVotes[msg.sender];
+
+        // check reveal period
+        require(vote.commitEndDate < getTimestamp());


Seems like we could aggregate these into a commitPeriodActive bool on UserVote

bingen · 2018-10-04T10:09:30Z

contracts/PLCR.sol

@@ -120,11 +120,11 @@ contract PLCR is AragonApp, IVoting {
        require(getTimestamp() <= vote.commitEndDate);

        // check lock and get amount
-        uint256 stake = checkLock(msg.sender, _lockId, vote.revealEndDate);
+        uint256 stake = checkLock(msg.sender, _lockId, vote.commitEndDate, vote.revealEndDate);


This is not enough: the user could unlock the remaining tokens (stake - slashStake) to another account before the reveal period starts (i.e., the commit period ends) and vote again.

First contract version

cba9d3a

bingen added component: contract epic: tcr new-app labels Jun 22, 2018

bingen requested review from izqui and sohkai June 22, 2018 17:33

bingen added 5 commits June 25, 2018 20:17

Unify interfaces

de487ae

Fix wrong used locks management

6f0dc68

Staking app registers locks per user, not globally.

Add stake to Commit and Reveal events

6b247ce

Fix bug in getVoteResult

dbc55b7

Computed result was not being used because of memory variable usage.

Unlock on reveal

b5dfa30

Besides, own usage lock register was not being freed.

bingen self-assigned this Jul 9, 2018

bingen added 2 commits July 24, 2018 19:40

Add function doc headers

c0357d5

Closes #2.

Adapt to last changes in Staking app

4ba4500

Locks now have start date too. Modified events in tests. `unlockAndMoveTokens` renamed to `unlockPartialAndMoveTokens`.

bingen force-pushed the contracts_first_version branch from 1248fb7 to 4ba4500 Compare July 24, 2018 17:40

izqui requested changes Aug 1, 2018

View reviewed changes

izqui reviewed Aug 2, 2018

View reviewed changes

izqui mentioned this pull request Aug 2, 2018

Staking 4 deferred lock aragon/aragon-apps#409

Closed

benstew reviewed Aug 9, 2018

View reviewed changes

bingen added this to the sprint: 1.2 milestone Sep 10, 2018

bingen commented Oct 4, 2018

View reviewed changes

sohkai removed epic: tcr labels Dec 10, 2018

sohkai removed this from the sprint: 1.2 milestone Jan 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First contract version #1

First contract version #1

bingen commented Jun 22, 2018

izqui left a comment

izqui Aug 1, 2018

izqui Aug 1, 2018

izqui Aug 1, 2018

izqui Aug 1, 2018

benstew Aug 9, 2018

izqui Aug 1, 2018

izqui Aug 1, 2018

izqui Aug 1, 2018

izqui Aug 1, 2018

izqui Aug 1, 2018

izqui Aug 1, 2018

benstew Aug 9, 2018

izqui Aug 1, 2018

izqui Aug 1, 2018

izqui Aug 2, 2018

benstew Aug 9, 2018

benstew Aug 9, 2018

benstew Aug 9, 2018

benstew Aug 9, 2018

benstew Aug 9, 2018

bingen Oct 4, 2018


		bytes32 constant public CREATE_VOTE_ROLE = keccak256("CREATE_VOTE_ROLE");

		// TODO!!!

		import "@aragon/apps-staking/contracts/interfaces/IStaking.sol";


		contract StakingMock is IStaking {

First contract version #1

Are you sure you want to change the base?

First contract version #1

Conversation

bingen commented Jun 22, 2018

izqui left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment